-
Notifications
You must be signed in to change notification settings - Fork 5
New eG Innovations Plugin #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New eG Innovations Plugin #11
Conversation
|
Adding a new plugin in squaredup |
…ations/squaredup-plugin-public into add-eginnovations-plugin
JSON parsing + other fixes
add error handling and better json handling
Updating from main
Syncup with main
…dd-eginnovations-plugin
…ations/squaredup-plugin-public into add-eginnovations-plugin if it merges an updated upstream into a topic branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requested changes are small ones and mostly around changes to our tooling. Otherwise there's nothing specific that concerns me!
In an advisory category:
You're importing objects but not (that I can see...) actually using them anywhere. You could drop these to simplify things (internally we don't import objects that we don't use without a good reason. If you have a good reason then please feel free to ignore me!).
You've got both axios and node-fetch as dependencies, but I don't think you're using axios at all. I'd suggest dropping that as a dependency, if only to make the bundle smaller and faster.
At the moment you're not using the report object in any capacity that I can see (just the log object). You can use report.error and report.warning to send back messages to the user in much the same way that you can use log.error to send messages to the log file. Something to think about for a future iteration maybe.
There seem to be a lot of label roles in your metadata, including on things like dates. We're normally a bit more targeted in our use of these roles so I'm not sure how this will behave (I think previously this could create excessively long tooltips on line graphs, but it's also been a long time since I last saw that issue so it may no longer be a thing).
|
A failing unit test to look into @deepalisingh-r : `FAIL packages/@squaredup/unit-test/dataStreamDefaults.test.js |
AnEvilPenguin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes!
|
Approving based on the test evidence provided |
Tenant to Deploy to: eG Innovations Pvt Ltd
Description
Adding a new plugin for squaredup on premises application.
PR Creation Checklist:
breakingornon-breakingbug-fix,new-plugin,internalorenhancement